Claude finished @tolgahan-arikan's task —— View job

PR Review: Align public error contracts

I reviewed the error-contract changes against the focus areas. Overall this is a well-structured, conservative change with strong test coverage and documentation that stays in sync with the code. Approving in spirit — only minor/optional notes below.

✅ Security (private key material / P-256 non-extractable)

No regressions. The diff never persists key material into session storage. Session expiry/clear paths (WalletSessions.swift:95-108, 220-239) continue to call credentialSession.clearSignerKeepingCredentials() / clear() and rebuild the signed client; they only touch wallet ID/address/metadata. The Keychain-backed signer remains the single owner of credential state. OIDC redirect state stays separate from completed session state, and restorePendingOidcRedirectAuth (WalletAuth.swift:307-317) still validates signerCredentialId + key type before resuming — good.

✅ No floating-point for token amounts

formatTokenAmount (WalletOperations.swift:494-498) delegates to formatUnits; zero-balance fallbacks use the string "0". No float math introduced.

✅ Platform availability

New public surfaces (OmsSdkError, OmsSdkErrorCode, OmsSdkOperation, OmsUpstreamService, OmsUpstreamError) are plain value types with no availability-gated APIs, consistent with how other pure model types in the SDK are declared. The internal runOmsOperation helpers correctly carry @available(macOS 12.0, iOS 15.0, *) (OmsSdkError.swift:153-177). No missing annotation found.

✅ Test coverage

PublicErrorContractsTests.swift is thorough — it exercises real public calls across WaaS transport/domain/HTTP/bad-response families, local session/selection/OIDC errors, indexer backend/transport/malformed paths, and the two transaction-boundary codes. The non-JSON sanitization assertions (:114-115, :581-582) are a nice touch verifying raw HTML bodies don't leak into message/upstreamError.

✅ Correctness — transaction write boundaries

The execute vs. polling distinction is implemented correctly:

• Execute failure → transactionExecutionUnconfirmed, retryable: false, preserves txnId (WalletOperations.swift:238-250).
• Post-submit polling failure → transactionStatusLookupFailed, retryable: true, preserves txnId (:401-413).
• toOmsSdkError preserves these via isNestedTransactionBoundary so the outer walletSendTransaction/walletCallContract wrapper doesn't overwrite them (OmsSdkError.swift:181, 411-415). Verified by the polling test which asserts operation == .walletTransactionStatus survives the outer wrapper.

Minor / optional observations (non-blocking)

1. Unused enum cases. OmsSdkErrorCode.walletSelectionInFlight (OmsSdkError.swift:12) and OmsSdkOperation.pendingWalletSelection (:19) are declared but never thrown/used in SDK code (walletTransactionStatus and the others are used). If these exist purely for Kotlin/TS parity, that's reasonable — but a one-line comment or a note in docs/error-contracts.md would prevent a future reader from assuming they're dead code. Worth confirming intent.

2. normalizedCode is computed but its companion isn't symmetric. In WebRPCError.toOmsSdkError (OmsSdkError.swift:266), the .invalidResponse branch condition kind == .webrpcBadResponse || kind == .unknown && code == ... relies on Swift precedence (&& binds tighter than ||). It's correct, but adding parentheses kind == .webrpcBadResponse || (kind == .unknown && code == WebRPCErrorKind.unknown.code) would make intent unambiguous.

3. Indexer transport upstream name. String(describing: type(of: error)) yields e.g. NSURLError for a URLError (asserted in test :598). This is fine and tested; just flagging that the name is Swift-runtime-derived rather than a stable service code, which the docs already acknowledge ("service-specific").

4. Docs/paths. No new shell examples with unquoted spacey paths were introduced in the README/API diffs. docs/error-contracts.md correctly references Tests/Swift SDKTests/PublicErrorContractsTests.swift as a path string (not a shell command), so quoting isn't required there.

⚠️ Could not run build/tests

swift build / swift test are not permitted in this review environment, so I could not independently verify compilation. The PR reports both pass, and CI (ci.yml) gates this. Please ensure the green CI run reflects the latest commit.

Summary: No security, floating-point, availability, or contract-correctness issues found. The four notes above are optional polish (mainly: clarify the two unused parity enum cases). Nice work keeping API.md, README.md, TESTING.md, and docs/error-contracts.md aligned with the implementation.
• align-error-contracts

@claude review

Claude finished @tolgahan-arikan's task —— View job

PR Review: Align public error contracts

• Read OmsSdkError.swift (WebRPC/WaaS mapping)
• Read IndexerClient.swift (error mapping)
• Validate Codex P2 findings
• Review transaction boundary + demo error handling
• Post review

I reviewed the latest commit (c8bc8ba, "address error contract review feedback") against the focus areas. This is a clean, well-tested change and the two Codex P2 findings appear to have already been resolved in c8bc8ba. Details below.

✅ Codex finding #1 — WaaS 5xx mapped as .httpError (resolved)

Codex reviewed 000d45e, where WebrpcServerPanic/WebrpcInternalError fell through to .requestFailed. In the current isHttpWebRPCError (Sources/Swift SDK/Models/OmsSdkError.swift:321-337) those kinds are now explicitly in the switch:

case .webrpcBadRoute, .webrpcBadMethod, .webrpcBadRequest,
     .webrpcBadResponse, .webrpcServerPanic, .webrpcInternalError:
    return true

So a WaaS WebRPC failure with a 4xx/5xx status now maps to .httpError with status set and retryable = status >= 500. Contract matches the docs. ✔️

✅ Codex finding #2 — local Indexer .invalidUrl misclassified (resolved)

In IndexerClient.postJson (Sources/Swift SDK/Clients/IndexerClient.swift:151-154) the catch now narrows to transport only:

} catch let error as HttpError {
    guard case .transport = error else { throw error }
    ...
}

Non-transport HttpError cases (.invalidUrl, .encodingFailed) re-throw and flow through runOmsOperation → toOmsSdkError → HttpError.toOmsSdkError, which maps them to .validationError (OmsSdkError.swift:393-399) — non-retryable and not attributed to the Indexer service. A malformed custom indexerGatewayUrl is now correctly a local validation failure. ✔️

✅ Security (P-256 / non-extractable credential)

No key material persisted into session storage anywhere in the diff. Session/expiry clearing continues to delegate to the Keychain-backed signer, and OIDC redirect state stays separate from completed session state. No regressions.

✅ No floating-point for token amounts

enrichFeeOptionsWithBalances zero-balance fallback uses the string "0" (WalletOperations.swift:357) and amounts go through formatTokenAmount/formatUnits. No float math.

✅ Platform availability

New public types (OmsSdkError, OmsSdkErrorCode, OmsSdkOperation, OmsUpstreamService, OmsUpstreamError) are pure value types; the internal runOmsOperation helpers carry @available(macOS 12.0, iOS 15.0, *) (OmsSdkError.swift:153,167). Consistent.

✅ Transaction write boundaries

Execute failure → .transactionExecutionUnconfirmed, retryable: false, preserves txnId (WalletOperations.swift:240-249). Post-submit polling failure → .transactionStatusLookupFailed, retryable: true, preserves txnId (:403-412). isNestedTransactionBoundary (OmsSdkError.swift:417-419) prevents the outer wrapper from overwriting these. Correct.

✅ Demo error handling

AppError.swift surfaces the new contract fields (operation, code, status, retryable, upstream) and tri-states retryable via retryableDescription (:108-117), matching the nullable contract. Good alignment with the demo-app guidance in AGENTS.md.

Minor / optional (non-blocking)

1. Parity-only enum cases. OmsSdkErrorCode.walletSelectionInFlight (OmsSdkError.swift:12) and OmsSdkOperation.pendingWalletSelection (:19) are declared but not thrown in SDK code. If they exist for Kotlin/TS parity, a one-line note in docs/error-contracts.md would stop a future reader from treating them as dead code. (Carried over from the prior review — still worth a quick confirmation of intent.)

2. Indexer retryable symmetry. validateSuccessResponse uses retryable: response.statusCode >= 500 (IndexerClient.swift:244), matching the WaaS HTTP path. Indexer transport errors are hard-coded retryable: true (:160). Consistent and reasonable; just noting the two intentionally differ for the non-status transport case.

3. String(describing: type(of: error)) for upstream name (IndexerClient.swift:256,263) yields a Swift-runtime-derived name (e.g. NSURLError) rather than a stable code. Docs already acknowledge this as "service-specific"; fine as-is.

⚠️ Could not run build/tests

swift build / swift test aren't available in this review environment, so I couldn't independently verify compilation. CI (ci.yml) gates this PR — please ensure the green run reflects c8bc8ba.

Summary: Both Codex P2 findings are already addressed in the latest commit. No security, floating-point, availability, or contract-correctness issues found — only optional polish (clarify the parity-only enum cases). Docs (API.md, README.md, TESTING.md, docs/error-contracts.md) stay in sync with the implementation. Nice work. ✅
• align-error-contracts